Conversation
bigbrett
force-pushed
the
async-crypto-v2
branch
from
6c7e032 to
7ef52c5
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an async (non-blocking) client crypto API for SHA-family operations by splitting each operation into *Request() and *Response() halves, while keeping existing blocking wrappers and the crypto-callback path compatible.
Changes:
- Adds async SHA-224/256/384/512 client APIs (plus DMA async variants) and supporting client DMA async bookkeeping.
- Updates SHA message wire formats to carry intermediate state plus variable-length trailing input, and refactors server handlers to be stateless per request.
- Expands crypto test coverage to exercise large-input, boundary, and async request/response flows; updates coverage tooling flags and POSIX example buffer sizing.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_message_crypto.h | Redefines SHA request/DMA request layouts to support variable-length input and inline state; adds per-call inline capacity macros. |
| wolfhsm/wh_client_crypto.h | Adds async Request/Response API declarations for SHA2 variants (and DMA variants), including bool request-sent signaling. |
| wolfhsm/wh_client.h | Adds per-client DMA async context storage for SHA to survive Request/Response boundary for POST cleanup. |
| src/wh_server_crypto.c | Updates server-side SHA/SHA-DMA handlers to consume new layouts, validate inSz, and operate statelessly. |
| src/wh_message_crypto.c | Updates translation functions for new SHA request/DMA request formats and expanded DMA response fields. |
| test/wh_test_crypto.c | Adds extensive new SHA tests (large input, capacity boundaries, async non-DMA + DMA). |
| test/wh_test_check_struct_padding.c | Updates padding/compile checks for renamed SHA DMA request structs. |
| test/Makefile | Adds gcovr parse-error ignore flag for coverage generation. |
| .github/workflows/code-coverage.yml | Mirrors gcovr parse-error ignore flag in CI coverage summary step. |
| examples/posix/wh_posix_server/wolfhsm_cfg.h | Updates example comm buffer size to 8 KiB and clarifies it must match client. |
| examples/posix/wh_posix_client/wolfhsm_cfg.h | Updates example comm buffer size to 8 KiB. |
| examples/posix/wh_posix_cfg.h | Updates SHM req/resp buffer sizing to accommodate larger comm packets. |
| docs/draft/async-crypto.md | Adds design doc describing the async crypto API and SHA wire protocol changes. |
| docs/draft/posix-shm.md | Adds design doc explaining POSIX SHM transport vs DMA feature vs allocator glue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bigbrett
requested review from
wolfSSL-Fenrir-bot
and removed request for
wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #337
Scan targets checked: wolfhsm-consttime, wolfhsm-crypto-bugs, wolfhsm-defaults, wolfhsm-mutation, wolfhsm-proptest, wolfhsm-src, wolfhsm-zeroize
Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
Adds an asynchronous (non-blocking) request/response API surface for SHA-family crypto operations in wolfHSM, while keeping existing blocking wrappers and the crypto-callback path compatible. This expands the wire protocol to carry resumable hash state + variable-length trailing input, and updates server routing accordingly.
Changes:
- Introduces
*Request()/*Response()async APIs for SHA-256/224/384/512 (DMA and non-DMA variants). - Updates SHA message wire layouts (inline variable-length input, resumable state, new DMA request/response structs) and server-side handling to be stateless per request.
- Adds extensive tests for large inputs and async flows; updates POSIX example buffer sizing/config and coverage tooling/docs.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_message_crypto.h | New SHA request wire formats (variable-length trailing input), new SHA DMA request/response structs, inline-capacity macros |
| wolfhsm/wh_client_crypto.h | Declares new async SHA Request/Response APIs (DMA + non-DMA) |
| wolfhsm/wh_client.h | Adds per-client DMA async context storage for POST cleanup across Request/Response |
| src/wh_server_crypto.c | Refactors SHA handlers to consume inSz trailing bytes and enforce invariants; updates SHA DMA handlers for new messages |
| src/wh_message_crypto.c | Updates message translation functions for new SHA structs (incl. DMA) |
| test/wh_test_crypto.c | Adds reference hashing + large-input and async tests for SHA variants (and DMA async tests under DMA) |
| test/wh_test_check_struct_padding.c | Updates padding-check declarations for new SHA DMA request structs |
| examples/posix/wh_posix_{client,server}/wolfhsm_cfg.h | Sets comm data length to 8KiB for POSIX examples |
| examples/posix/wh_posix_cfg.h | Increases SHM transport request/response buffer sizes to match larger comm payloads |
| docs/draft/async-crypto.md | Design doc describing async crypto API and SHA wire protocol |
| docs/draft/posix-shm.md | Design doc explaining POSIX SHM transport + DMA layering |
| test/Makefile | Adds gcovr parse-error ignore option to coverage target |
| .github/workflows/code-coverage.yml | Mirrors the gcovr ignore option in CI coverage summary step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rizlik
left a comment
rizlik
left a comment
There was a problem hiding this comment.
Thanks for the clear PR and docs.
My main feedback/concerns:
a) absence of a way for the client to track the in-flight request. PR acknowledge that, nevertheless it should be one of the main design choices of the API. The PR should either add it here or at least sketch the design in the docs. I don't think offloading the management of the req/resp tracking (is a request in flight? what kind? etc) fully to the application is a good design. At least we should protect the application and error out on bad API usage (eg, two request() without response(), request() and response() of mismatching type, etc).
b) redundancy of the code in wh_client_crypto.c: this is a pre-existing problem for sure, but can we reduce the amount of almost exact code across both sha variants and DMA vs non-DMA paths?
c) API usability: how the app knows the outbound limit? (see comments below). Should we switch on blocking behavior for large input size? or should we add a specific error message? or should we expose the function that compute the outbound limit?
Is using a single API (*Request or a more general *Operation) instead of two (*Request/*Response) worth considering?
It will solve also the cumbersome requestSent (the API can return WH_ERROR_OK directly), and it can address (c) by chunking at the wolfHSM layer big request across multiple client <-> server roundtrips.
In this case the app just keep calling the API until an ret != WH_ERROR_NOTREADY, following a POSIX-like semantic.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Only smaller issues building on top of Marco's comments.
|
@rizlik @Frauschi @AlexLanzano Thanks all for the feedback! Believe I addressed all the pending comments in my last few commits - pending the items we discussed punting. The big change is we now track request in flight and have a test for it, making sure the client APIs should error out if misused. |
Async Crypto API for SHA
Introduces a non-blocking request/response split for wolfHSM crypto operations, starting with the SHA family. Every blocking call gains a
*Request()/*Response()pair;Request()sends and returns immediately,Response()polls once and returnsWH_ERROR_NOTREADYuntil the server replies. Existing blocking wrappers and thewh_Client_CryptoCbpath are unchanged, so current application code keeps working.Summary
wh_client_crypto.hcovering SHA-1/224/256/384/512wh_client_crypto.caround shared request/response primitiveswh_message_crypto.{c,h}to carry async-specific state (intermediate hash state, trailing input, DMA metadata)wh_server_crypto.c(requests handled statelessly per design)docs/draft/async-crypto.md(includes roadmap for remaining algorithms)test/wh_test_crypto.ccode-coverage.ymlFuture work